Update CMIP6 and obs4mips cmor tables to last release#158
Conversation
|
Thanks, I think this helps a lot! There is a problem that we are slowly approaching: CMIP6 data will not follow a uniform use Both of them do so following the The oldest I am not sure what, if anything, to do about it right now, but at some point we will get this kind of error.
What do you think? |
I think this is not possible
This will be a nightmare, not only for us but for users. They will find different CMIP6 specifications in their diagnostic code and will have to adapt to it.
I prefer this, with one condition: we choose last version released and keep updating. We should assume that changes are done for a reason. Most variables will be the same in all versions anyway, so I think the number of fixes will be manageable. We can add fixes at In your example, we can add a |
|
Ok, @jvegasbsc I agree with that approach. I opened a new issue for that (#159) so that we can finish this PR. Thanks! |
bouweandela
left a comment
There was a problem hiding this comment.
It looks like frequency is now always defined in the variable and never in the table header. Could you simplify the code so it only reads the frequency from the variable section, i.e. remove the frequency related code from CMIP6Info._load_table and add it in VariableInfo.read_json?
|
@valeriupredoi I believe you worked on this topic while @jvegasbsc was on parental leave. Can you also have a look at this pull request? |
|
There is also this code ESMValCore/esmvalcore/_recipe.py Lines 127 to 129 in d94e6a7 which will not work anymore for CMIP6 tables. I'm not entirely sure why it is here anyway, but it seems somehow related to derived variables. |
Out of the box it will not work, but we are assigning a frequency to the mip by ourselves: the most common in the variables.
Yes, it is for assigning a frequency for the checks for custom and derived variables |
There are other projects that are be using CMOR3 format for their tables (i.e. PRIMAVERA) I don't know if all of them made this change. By the way, we are using CMIP5 and CMIP6 as cmor types when we are really refering to CMOR 2 and CMOR 3. I think it is easier for users to match, but the naming is not entirely correct |
Would it be possible to replace ESMValCore/esmvalcore/_recipe.py Lines 116 to 120 in d94e6a7 ESMValCore/esmvalcore/_recipe.py Lines 127 to 129 in d94e6a7 by something like cmor_strict = False if derive else 'default'
table_entry = CMOR_TABLES[cmor_table].get_variable(mip, short_name, cmor_strict)and change the table readers so this works (i.e. if 'default' is passed it should use whatever is configured in config-developer.yml)? I think it's confusing to change the content of the cmor variable definitions outside of the cmor table module and also using the |
I don't see any code that does this, can you point me to it? |
Here: https://github.com/ESMValGroup/ESMValTool/pull/985/files I thought this was merged, but it seems it got lost in the split |
|
Notice how that PR actually contains updated tables. |
|
yes, my old PR! And now @mattiarighi is busting my balls about it 😁 |
well, they were the latest at their time (the time of the PR), now they've become obsolete as well, I believe the latest CMOR version is 01.00.30 (if not 31beta) |
|
@jvegasbsc are you going to port ESMValGroup/ESMValTool#985 in here? |
|
Yes, I will start working on it now |
… into dev_update_cmip6_tables
|
Please do not merge this until is tested with several recipes. |
|
Re Mattias concern: In principle, the headaches should be reduced this time round simply because of the split. No change to the master branch here impacts recipe development, so I wonder if we should not rather get this merged relatively quickly, but make sure that things work or at least that there is a clear way to adjust existing recipes, only before we produce the next (pre-)release? Also, there are very few CMIP6 recipes in the public part of ESMValTool at the moment. Which recipes do you think should be checked? |
That's what I meant, just run a few recipes to make sure nothing gets broken.
Some of the ones using So just give me your OK when this is ready for testing. |
Hah, what a sneaky way to quote :) The important bit for me was of course
Sure, but what does stable mean? We will want to introduce changes that break existing recipes. That's ok and should not in and of itself delay a release of the core, nor should the core be considered unstable because of it. Rather the ESMValTool should be judicious in adjusting the dependency on a new version of the core.
Great, that should definitely work for this PR. 👍 |
Yes, but not at this stage. But as I said, the usual testing procedure plus a couple of more recipes with CMIP6 models should be enough. |
|
The flip side of course is that we surely won't want to release with But yeah, point taken, smooth development experience is important now. |
|
Any update on this? |
valeriupredoi
left a comment
There was a problem hiding this comment.
if it works (as it looks like it does), me happy 🍺
Would like to see this go in. At the moment though, there are a bunch of open conversations that should be resolved one way or another first. |
… into dev_update_cmip6_tables
… into dev_update_cmip6_tables
|
Ready to merge from my side |
bouweandela
left a comment
There was a problem hiding this comment.
Code looks good, but I did not test.
|
@schlunma can you test it with one of your cmip6 recipes? |
|
On my way to work, shall do first thing
Dr Valeriu Predoi.
Computational scientist
NCAS-CMS
University of Reading
Department of Meteorology
Reading RG6 6BB
United Kingdom
…On Fri, 6 Sep 2019, 10:40 Klaus Zimmermann, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In esmvalcore/cmor/table.py
<#158 (comment)>
:
> self._assign_dimensions(var, generic_levels)
table[var_name] = var
+ self.var_to_freq[table.name][var_name] = var.frequency
+
+ if not table.frequency:
@valeriupredoi <https://github.com/valeriupredoi> care to answer and/or
resolve (in the github sense) this conversation?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158?email_source=notifications&email_token=AG5EFI5N5XHVXYN5KFJLGZTQIIQQHA5CNFSM4IA35HQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCD4RR2I#discussion_r321657249>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG5EFI6XXUW26FA2EM3QKADQIIQQHANCNFSM4IA35HQQ>
.
|
schlunma
left a comment
There was a problem hiding this comment.
Successfully tested with 3 recipes with ~16 CMIP6 models each (included CMIP5 and OBS data as well). I did not run diagnostic scripts, though.
Updating tables to last release
https://github.com/PCMDI/cmip6-cmor-tables/releases/tag/6.5.29
Applied a fix to Efx tables (generic level missing). It is already on master, but I prefer to use tags for this so it is easier to know the exact version we use
Closes #150 and closes #45
As a bonus, I added a better error message when trying to get a coordinate that is not defined